Allow multiple signing keys to be provided #4632

stephen-crawford · 2024-08-09T18:57:31Z

Description

[Describe what this change achieves]

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
This change is an enhancement which allows users to provide multiple signing keys when configuring JWT options.
Previously, users could only specify a single signing key when configuring their JWT options. As a result, if a user wanted to rotate their key, they would experience some amount of downtime since the cluster would have to reinitialize the update security configuration. This change does not stop the required bouncing, but because you can now provide multiple keys, you can provide backup(s) key(s) which means even when the cluster bounces you have the ability to send JWT requests with the backup key(s).

To specify the multiple keys, you just need to use the same method as before but separate the keys with a comma i.e. <key 1>, <key 2>, etc....

Issues Resolved

[List any issues this PR will resolve]
This PR addresses: #4613

Is this a backport? If so, please add backport PR # and/or commits #, and remove backport-failed label from the original PR.

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? If so, please open a draft PR in the security dashboards plugin and link the draft PR here
No

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]
I added several new tests to the existing HTTPJWTAuthenticatorTest class.

Check List

New functionality includes testing
New functionality has been documented
~~New Roles/Permissions have a corresponding security dashboards plugin PR~~
API changes companion pull request created
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Stephen Crawford <[email protected]>

codecov · 2024-08-09T19:21:44Z

Codecov Report

Attention: Patch coverage is 73.68421% with 10 lines in your changes missing coverage. Please review.

Project coverage is 65.22%. Comparing base (ec9fadb) to head (f5c708f).
Report is 8 commits behind head on main.

Files	Patch %	Lines
...mazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java	72.22%	6 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4632      +/-   ##
==========================================
- Coverage   65.26%   65.22%   -0.05%     
==========================================
  Files         317      317              
  Lines       22311    22319       +8     
  Branches     3588     3590       +2     
==========================================
- Hits        14562    14558       -4     
- Misses       5954     5964      +10     
- Partials     1795     1797       +2

Files	Coverage Δ
...in/java/org/opensearch/security/util/KeyUtils.java	`87.50% <100.00%> (-9.28%)`	⬇️
...mazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java	`72.56% <72.22%> (-1.02%)`	⬇️

... and 2 files with indirect coverage changes

src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java

Signed-off-by: Stephen Crawford <[email protected]>

src/test/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticatorTest.java

src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java

Signed-off-by: Stephen Crawford <[email protected]>

src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java

cwperks · 2024-08-19T15:49:17Z

Thanks for creating a documentation issue as well. Do you plan to open a PR along with the issue?

src/main/java/org/opensearch/security/util/KeyUtils.java

opensearch-trigger-bot · 2024-08-20T14:47:54Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-4632-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e2cd6101bf2d4f6c4877ec4692ed89be19af2d3b
# Push it to GitHub
git push --set-upstream origin backport/backport-4632-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-4632-to-2.x.

…o be provided) (#4666) Signed-off-by: Stephen Crawford <[email protected]>

krishna-ggk

Should we add a BWC test to ensure there is no breakage since the type of config is changing from singleton to list?

cwperks · 2024-08-23T13:27:13Z

@krishna-ggk Settings.getAsList is backwards compatible with Settings.get(). The same yaml configuration for jwt can be used with both Settings.get and Settings.getAsList for the signing_key portion of the config:

Example jwt config:

jwt_auth_domain:
    description: "Authenticate via Json Web Token"
    http_enabled: false
    transport_enabled: false
    order: 0
    http_authenticator:
      type: jwt
      challenge: false
      config:
        signing_key: "base64 encoded HMAC key or public RSA/ECDSA pem key"
        jwt_header: "Authorization"
        jwt_url_parameter: null
        roles_key: null
        subject_key: null

One potential issue that could arise would be trying to update the JWT authc config in a mixed cluster of 2.17 and <2.17 nodes.

If the config update hit the 2.17 node and updated the jwt config then the <2.17 nodes would not know how to parse the new config.

Edit:

Settings.getAsList() supports either list entries or comma-separate entries in yaml.

All of these configs work:

signing_key:
  - <signing_key1>
  - <signing_key2>
  - <signing_key3>

signing_key: "<signing_key1>,<signing_key2>,<signing_key3>"

signing_key: "<signing_key1>"

krishna-ggk · 2024-08-26T13:21:25Z

Thanks @cwperks for clarifying the backward compatibility aspects.

If the config update hit the 2.17 node and updated the jwt config then the <2.17 nodes would not know how to parse the new config.

Does the config update through API generate list or CSV? If so, it could potentially cause issue by generating incompatible version even if only single value is provided. Admittedly though this should be a remote scenario.

Potentially the least riskiest approach would be to introduce a pluralized attribute (signing_keys) which comes at expense of managing deprecation of older attribute. In interest of keeping things simple, I agree current approach is good enough.

Allow multiple signing keys to be provided

56e7b59

Signed-off-by: Stephen Crawford <[email protected]>

stephen-crawford requested review from cliu123, cwperks, DarshitChanpura, peternied, RyanL1997, reta and willyborankin as code owners August 9, 2024 18:57

Allow multiple signing keys to be provided

5cfac35

Signed-off-by: Stephen Crawford <[email protected]>

derek-ho added the backport 2.x backport to 2.x branch label Aug 12, 2024

cwperks reviewed Aug 13, 2024

View reviewed changes

src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java Outdated Show resolved Hide resolved

stephen-crawford added 2 commits August 13, 2024 13:13

Swap to get setting as list

75c69b1

Signed-off-by: Stephen Crawford <[email protected]>

Swap to get setting as list

d6ed2fb

Signed-off-by: Stephen Crawford <[email protected]>

cwperks reviewed Aug 13, 2024

View reviewed changes

src/test/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticatorTest.java Show resolved Hide resolved

cwperks reviewed Aug 14, 2024

View reviewed changes

src/test/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticatorTest.java Show resolved Hide resolved

cwperks previously approved these changes Aug 14, 2024

View reviewed changes

shikharj05 reviewed Aug 16, 2024

View reviewed changes

src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java Show resolved Hide resolved

stephen-crawford and others added 2 commits August 16, 2024 10:37

Merge branch 'opensearch-project:main' into allowMultipleKeys

88d13d2

Allow multiple signing keys to be provided

17dcd76

Signed-off-by: Stephen Crawford <[email protected]>

stephen-crawford dismissed cwperks’s stale review via 17dcd76 August 16, 2024 14:44

stephen-crawford added 3 commits August 16, 2024 10:44

Allow multiple signing keys to be provided

d745193

Signed-off-by: Stephen Crawford <[email protected]>

Allow multiple signing keys to be provided

54cd4e0

Signed-off-by: Stephen Crawford <[email protected]>

Allow multiple signing keys to be provided

f5c708f

Signed-off-by: Stephen Crawford <[email protected]>

stephen-crawford mentioned this pull request Aug 16, 2024

[DOC] Update JWT signing key setting use opensearch-project/documentation-website#8039

Closed

4 tasks

cwperks reviewed Aug 19, 2024

View reviewed changes

src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java Show resolved Hide resolved

cwperks approved these changes Aug 19, 2024

View reviewed changes

DarshitChanpura approved these changes Aug 19, 2024

View reviewed changes

src/main/java/org/opensearch/security/util/KeyUtils.java Show resolved Hide resolved

shikharj05 approved these changes Aug 20, 2024

View reviewed changes

stephen-crawford merged commit e2cd610 into opensearch-project:main Aug 20, 2024
41 of 42 checks passed

opensearch-trigger-bot bot added the backport-failed label Aug 20, 2024

cwperks added the v2.17.0 Issues targeting release v2.17.0 label Aug 20, 2024

stephen-crawford mentioned this pull request Aug 20, 2024

[Backport 2.x] Manually backport #4632 (Allow multiple signing keys to be provided) #4666

Merged

4 tasks

stephen-crawford added a commit that referenced this pull request Aug 20, 2024

[Backport 2.x] Manually backport #4632 (Allow multiple signing keys t…

6c431e1

…o be provided) (#4666) Signed-off-by: Stephen Crawford <[email protected]>

krishna-ggk reviewed Aug 23, 2024

View reviewed changes

rursprung mentioned this pull request Nov 15, 2024

[BUG] regression in 2.17: error log on each request when multiple JWT auth domains are configured #4910

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple signing keys to be provided #4632

Allow multiple signing keys to be provided #4632

stephen-crawford commented Aug 9, 2024 •

edited

Loading

codecov bot commented Aug 9, 2024 •

edited

Loading

cwperks commented Aug 19, 2024

opensearch-trigger-bot bot commented Aug 20, 2024

krishna-ggk left a comment

cwperks commented Aug 23, 2024 •

edited

Loading

krishna-ggk commented Aug 26, 2024

Allow multiple signing keys to be provided #4632

Allow multiple signing keys to be provided #4632

Conversation

stephen-crawford commented Aug 9, 2024 • edited Loading

Description

Issues Resolved

Testing

Check List

codecov bot commented Aug 9, 2024 • edited Loading

Codecov Report

cwperks commented Aug 19, 2024

opensearch-trigger-bot bot commented Aug 20, 2024

krishna-ggk left a comment

Choose a reason for hiding this comment

cwperks commented Aug 23, 2024 • edited Loading

krishna-ggk commented Aug 26, 2024

stephen-crawford commented Aug 9, 2024 •

edited

Loading

codecov bot commented Aug 9, 2024 •

edited

Loading

cwperks commented Aug 23, 2024 •

edited

Loading